Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

introduce typescript #447

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

introduce typescript #447

wants to merge 1 commit into from

Conversation

jgdhs27
Copy link
Collaborator

@jgdhs27 jgdhs27 commented Aug 13, 2023

Adding types here to make the logic easier to follow

Copy link
Owner

@n-rook n-rook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice; using ts will be really convenient and make it easier to maintain the frontend without getting lost in the complexity of it.

@@ -42,6 +42,7 @@
<script>
const showGameColumn = "{{show_game}}";
const showRouteColumn = "{{show_route}}";
const aaaa = "aaaaa";
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

@@ -1,205 +1,200 @@
"use strict";
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to be annoying by saying this, but: We really shouldn't check in generated code. Rather, we should recreate it whenever we deploy.

If you really need to, it might be okay to check it in as an intermediate step while you're trying to get the ts deploy working, but if so, please put a giant comment at the top warning that this is generated code. But it'd be way better to do it in one go.

}

function ensureTickboxSync(): void {
activeFilters = initializeFilters();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Er--- why are we calling initializeFilters() three times in this module?

@@ -23,38 +23,38 @@ def convert_replay_to_dict(self, replay: models.Replay) -> dict:
score_prefix = _get_medal_emoji(replay.rank) if hasattr(replay, "rank") else ""

json_dict = {}
json_dict["Id"] = replay.id
json_dict["id"] = replay.id
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO you should really make these changes in a separate PR; they make this one look much bigger than it needs to be

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants